Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request adds package selection to the search results UI. A new composable Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
app/composables/usePackageSelection.ts (1)
1-32: Clean composable implementation.The use of
useStateensures SSR-safe shared state across components. The immutable array operations (filter, spread) correctly trigger Vue's reactivity.One consideration: there's no maximum selection limit enforced here. If the compare feature has a package limit (as suggested by
compare.selector.packages_selectedmentioning{count}/{max}), you may want to enforce it intogglePackageSelectionto prevent users from selecting more packages than can be compared.test/nuxt/a11y.spec.ts (1)
3323-3347: Cover the selected-state render path, not only the empty/default path.Both new tests mount without arranging selection data, so they mostly validate non-interactive states. Please add at least one case with selected packages to audit the actual action controls and announcements.
Based on learnings: Applies to **/*.{test,spec}.{ts,tsx} : Write unit tests for core functionality using
vitest.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
app/components/BaseCard.vueapp/components/ColumnPicker.vueapp/components/Package/ActionBar.vueapp/components/Package/Card.vueapp/components/Package/List.vueapp/components/Package/ListToolbar.vueapp/components/Package/SelectionView.vueapp/components/Package/Table.vueapp/components/Package/TableRow.vueapp/composables/usePackageSelection.tsapp/pages/search.vuei18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonshared/types/preferences.tstest/nuxt/a11y.spec.ts
| <button @click="clearSelectedPackages" class="flex items-center ms-2 hover:text-fg-muted"> | ||
| <span class="i-lucide:x text-xs relative top-px" aria-label="Close action bar" /> | ||
| </button> | ||
| </div> | ||
|
|
||
| <LinkBase | ||
| :to="{ name: 'compare', query: { packages: selectedPackagesParam } }" | ||
| variant="button-secondary" | ||
| classicon="i-lucide:git-compare" | ||
| > | ||
| Compare | ||
| </LinkBase> |
There was a problem hiding this comment.
The action-bar controls need accessible labelling and localisation cleanup.
Line 40-42 uses an icon-only close button without a button-level label, and Line 50 hardcodes visible text (Compare) instead of using i18n.
✅ Suggested patch
- <button `@click`="clearSelectedPackages" class="flex items-center ms-2 hover:text-fg-muted">
- <span class="i-lucide:x text-xs relative top-px" aria-label="Close action bar" />
+ <button
+ type="button"
+ :aria-label="$t('common.close')"
+ `@click`="clearSelectedPackages"
+ class="flex items-center ms-2 hover:text-fg-muted"
+ >
+ <span class="i-lucide:x text-xs relative top-px" aria-hidden="true" />
</button>
@@
- Compare
+ {{ $t('package.links.compare') }}
</LinkBase>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button @click="clearSelectedPackages" class="flex items-center ms-2 hover:text-fg-muted"> | |
| <span class="i-lucide:x text-xs relative top-px" aria-label="Close action bar" /> | |
| </button> | |
| </div> | |
| <LinkBase | |
| :to="{ name: 'compare', query: { packages: selectedPackagesParam } }" | |
| variant="button-secondary" | |
| classicon="i-lucide:git-compare" | |
| > | |
| Compare | |
| </LinkBase> | |
| <button | |
| type="button" | |
| :aria-label="$t('common.close')" | |
| `@click`="clearSelectedPackages" | |
| class="flex items-center ms-2 hover:text-fg-muted" | |
| > | |
| <span class="i-lucide:x text-xs relative top-px" aria-hidden="true" /> | |
| </button> | |
| </div> | |
| <LinkBase | |
| :to="{ name: 'compare', query: { packages: selectedPackagesParam } }" | |
| variant="button-secondary" | |
| classicon="i-lucide:git-compare" | |
| > | |
| {{ $t('package.links.compare') }} | |
| </LinkBase> |
| <span class="sr-only"> | ||
| {{ $t('package.card.select') }} | ||
| </span> |
There was a problem hiding this comment.
Make checkbox accessible names package-specific.
Each checkbox currently announces the same label (“Select package”), which is ambiguous when navigating multiple results with assistive tech. Include the package name in the accessible label.
💡 Suggested tweak
- <span class="sr-only">
- {{ $t('package.card.select') }}
- </span>
+ <span class="sr-only">
+ {{ `${$t('package.card.select')}: ${result.package.name}` }}
+ </span>| <button @click="clearSelectedPackages" class="flex items-center ms-2"> | ||
| <span class="i-lucide:x text-sm" aria-label="Close action bar" /> | ||
| </button> |
There was a problem hiding this comment.
Clear-selection icon button is missing a reliable accessible name.
Line 229 renders an icon-only button, but the label is on the child <span> (Line 230), not on the <button> itself.
✅ Suggested patch
- <button `@click`="clearSelectedPackages" class="flex items-center ms-2">
- <span class="i-lucide:x text-sm" aria-label="Close action bar" />
+ <button
+ type="button"
+ :aria-label="t('common.close')"
+ `@click`="clearSelectedPackages"
+ class="flex items-center ms-2"
+ >
+ <span class="i-lucide:x text-sm" aria-hidden="true" />
</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button @click="clearSelectedPackages" class="flex items-center ms-2"> | |
| <span class="i-lucide:x text-sm" aria-label="Close action bar" /> | |
| </button> | |
| <button | |
| type="button" | |
| :aria-label="t('common.close')" | |
| `@click`="clearSelectedPackages" | |
| class="flex items-center ms-2" | |
| > | |
| <span class="i-lucide:x text-sm" aria-hidden="true" /> | |
| </button> |
| defineProps<{ | ||
| viewMode?: ViewMode | ||
| }>() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify ViewMode import status in the target file and check for global declarations
rg -n "defineProps<\{|viewMode\?: ViewMode|import.*ViewMode|export.*ViewMode" app/components/Package/SelectionView.vue
# Check for global ViewMode declarations
rg -n "type ViewMode|interface ViewMode|declare.*ViewMode" --type ts --type d.tsRepository: npmx-dev/npmx.dev
Length of output: 146
🏁 Script executed:
# Read the full SelectionView.vue file to see all imports
cat -n app/components/Package/SelectionView.vue
# Search for ViewMode type definition globally with proper patterns
rg "type ViewMode|interface ViewMode" --type ts
# Check if the suggested import path exists
fd "preferences" --type fRepository: npmx-dev/npmx.dev
Length of output: 1753
🏁 Script executed:
# Search for TypeScript/Nuxt configuration to verify the `#shared` alias
fd "tsconfig\|nuxt\.config\|vite\.config" --type f
# Check for any global type declarations
rg "declare global|declare module|ambient" --type ts
# Search for any .d.ts files at root or shared directories
find . -name "*.d.ts" -type f | head -20Repository: npmx-dev/npmx.dev
Length of output: 514
🏁 Script executed:
# Find and inspect TypeScript/Nuxt config files
cat -n tsconfig.json 2>/dev/null || echo "tsconfig.json not found"
cat -n nuxt.config.ts 2>/dev/null || echo "nuxt.config.ts not found"
# Also check for alias resolution in vite config or other config
find . -maxdepth 2 -name "nuxt.config.*" -o -name "tsconfig.json" | head -5Repository: npmx-dev/npmx.dev
Length of output: 13729
🏁 Script executed:
# Check if Nuxt auto-generates `#shared` alias by looking for nuxt auto-import config
# or check if there's any auto-import configuration for types
rg "auto|#shared" nuxt.config.ts
# Verify if ViewMode is used elsewhere and how it's imported
rg "ViewMode" --type ts --type vue | head -30Repository: npmx-dev/npmx.dev
Length of output: 145
🏁 Script executed:
# Check how ViewMode is imported in other files
rg "import.*ViewMode" --type ts
# Also check the app's tsconfig to see if it has the `#shared` alias
cat -n .nuxt/tsconfig.app.json 2>/dev/null | head -50Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check all files in .nuxt directory that might have path aliases
find .nuxt -name "tsconfig*.json" -type f | head -10
# Check if any tsconfig actually has the `#shared` path
rg "#shared" --type ts
# Let's look for any import of ViewMode in the entire codebase
rg "ViewMode" | grep -i importRepository: npmx-dev/npmx.dev
Length of output: 18137
ViewMode type is unresolved—add missing import.
The defineProps and template usage reference ViewMode without an import. The type is exported from #shared/types/preferences and must be imported locally in this file.
<script setup lang="ts">
+import type { ViewMode } from '#shared/types/preferences'
+
defineProps<{
viewMode?: ViewMode
}>()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defineProps<{ | |
| viewMode?: ViewMode | |
| }>() | |
| <script setup lang="ts"> | |
| import type { ViewMode } from '#shared/types/preferences' | |
| defineProps<{ | |
| viewMode?: ViewMode | |
| }>() |
| <th class="w-8"> | ||
| <span class="sr-only">{{ getColumnLabel('selection') }}</span> | ||
| </th> |
There was a problem hiding this comment.
Selection header introduces a loading-state column mismatch.
After adding this <th>, skeleton rows still render fewer <td> cells, so the table structure is inconsistent while loading.
💡 Suggested patch
- <th class="w-8">
+ <th scope="col" class="w-8">
<span class="sr-only">{{ getColumnLabel('selection') }}</span>
</th> <template v-if="isLoading && results.length === 0">
<tr v-for="i in 5" :key="`skeleton-${i}`" class="border-b border-border">
@@
<td v-if="isColumnVisible('keywords')" class="py-3 px-3">
<div class="h-4 w-32 bg-bg-muted rounded animate-pulse ms-auto" />
</td>
+ <td class="py-3 px-3 w-8">
+ <div class="h-4 w-4 bg-bg-muted rounded animate-pulse ms-auto" />
+ </td>
</tr>
</template>| <button | ||
| v-if="isSelectioView" | ||
| type="button" | ||
| class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" | ||
| @click="hideSelectionView" | ||
| > | ||
| <span class="i-lucide:arrow-left rtl-flip w-4 h-4" aria-hidden="true" /> | ||
| <span class="hidden sm:inline">{{ $t('nav.back') }}</span> | ||
| </button> |
There was a problem hiding this comment.
Back button needs an explicit accessible name, and should use the global focus-visible rule.
On small screens, Line 577 hides the visible text while Line 576 icon is aria-hidden, so the control may be unnamed. Also, Line 573 adds a per-element focus-visible utility on a <button>.
✅ Suggested patch
<button
v-if="isSelectioView"
type="button"
- class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
+ :aria-label="$t('nav.back')"
+ class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0"
`@click`="hideSelectionView"
>Based on learnings: In the npmx.dev project, focus-visible styling for button and select elements is implemented globally in app/assets/main.css; avoid per-element inline focus-visible utility classes on those elements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| v-if="isSelectioView" | |
| type="button" | |
| class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" | |
| @click="hideSelectionView" | |
| > | |
| <span class="i-lucide:arrow-left rtl-flip w-4 h-4" aria-hidden="true" /> | |
| <span class="hidden sm:inline">{{ $t('nav.back') }}</span> | |
| </button> | |
| <button | |
| v-if="isSelectioView" | |
| type="button" | |
| :aria-label="$t('nav.back')" | |
| class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0" | |
| `@click`="hideSelectionView" | |
| > | |
| <span class="i-lucide:arrow-left rtl-flip w-4 h-4" aria-hidden="true" /> | |
| <span class="hidden sm:inline">{{ $t('nav.back') }}</span> | |
| </button> |
| | 'maintenanceScore' | ||
| | 'combinedScore' | ||
| | 'security' | ||
| | 'selection' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if selection column is used in column visibility checks anywhere
rg -n "isColumnVisible.*selection" --type vue
rg -n "selection.*visible" --type tsRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Let's look at the actual file structure
find . -name "preferences.ts" -type f | head -5Repository: npmx-dev/npmx.dev
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Look at the DEFAULT_COLUMNS structure and surrounding context
rg -B 5 -A 50 "DEFAULT_COLUMNS" --type ts | head -100Repository: npmx-dev/npmx.dev
Length of output: 5575
🏁 Script executed:
#!/bin/bash
# Check ColumnPicker.vue for columnLabels usage with 'selection'
fd "ColumnPicker.vue" --type f -x cat -n {} | head -150Repository: npmx-dev/npmx.dev
Length of output: 5508
🏁 Script executed:
#!/bin/bash
# Check TableRow.vue for selection column rendering
fd "TableRow.vue" --type f -x cat -n {} | head -200Repository: npmx-dev/npmx.dev
Length of output: 8063
🏁 Script executed:
#!/bin/bash
# Search for selection checkbox rendering logic
rg -n "selection\|checkbox\|togglePackageSelection" --type ts --type tsx -B 2 -A 2 | head -80Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Look for the full TableRow.vue template to see if selection is rendered separately
fd "TableRow.vue" --type f -x cat -n {} | tail -100Repository: npmx-dev/npmx.dev
Length of output: 3876
🏁 Script executed:
#!/bin/bash
# Check the ColumnId type definition to see what it includes
rg -B 5 -A 15 "type ColumnId" shared/types/preferences.tsRepository: npmx-dev/npmx.dev
Length of output: 431
Remove unnecessary selection label from ColumnPicker.vue.
The selection column is added to the ColumnId type correctly. However, the selection entry in columnLabels (ColumnPicker.vue, line 57) is unused. Since selection is not included in DEFAULT_COLUMNS, it will never appear in the toggleable columns list. The selection checkbox is rendered as a separate table cell in TableRow.vue (lines 204–219) and is always visible, not controlled through the column visibility system. Remove the selection entry from columnLabels to avoid dead code.
🔗 Linked issue
resolves #1509
🧭 Context
Added a multi-select feature to the search page that allows users to select multiple packages and perform bulk actions on them. Currently supports comparing selected packages, with the possibility of adding more actions in the future.
📚 Description
Screen.Recording.2026-02-27.at.18.33.58.mov